Reviewable status: 0 of 23 files reviewed, 20 unresolved discussions (waiting on @gitlw, @manishrjain, @danielmai, @codexnull, and @srfrog)

dgraph/cmd/alpha/run.go, line 126 at r1 (raw file):

		"Commits to disk will give up after these number of retries to prevent locking the worker"+
			" in a failed state. Use -1 to retry infinitely.")
	flag.String("auth_token", "",

Keep this as an open-source feature.

dgraph/cmd/alpha/run.go, line 128 at r1 (raw file):

	flag.String("hmac_secret_file", "", "The file storing the HMAC secret"+
		" that is used for signing the JWT. Enterprise feature.")
	flag.Duration("access_jwt_ttl", 6*time.Hour, "The TTL for the access jwt. "+

Add an acl prefix to all these flags: acl_access_ttl

dgraph/cmd/alpha/run.go, line 130 at r1 (raw file):

	flag.Duration("access_jwt_ttl", 6*time.Hour, "The TTL for the access jwt. "+
		"Enterprise feature.")
	flag.Duration("refresh_jwt_ttl", 30*24*time.Hour, "The TTL for the refresh jwt. "+

acl_refresh_ttl. You can drop the jwt phrase.

dgraph/cmd/alpha/run.go, line 132 at r1 (raw file):

	flag.Duration("refresh_jwt_ttl", 30*24*time.Hour, "The TTL for the refresh jwt. "+
		"Enterprise feature.")
	flag.Duration("acl_refresh_interval", 30*time.Second, "The interval to refresh the acl cache. "+

acl_cache_ttl in keeping with the other flags.

dgraph/cmd/alpha/run.go, line 407 at r1 (raw file):

	secretFile := Alpha.Conf.GetString("hmac_secret_file")
	if secretFile != "" {
		if !Alpha.Conf.GetBool("enterprise_features") {

Consider this as a suggestion:

I think all this should lie within the acl code in ee/acl directory. You can call it like so:

acl.Init(Alpha.Conf)

dgraph/cmd/alpha/run.go, line 512 at r1 (raw file):

	// Setup external communication.
	go worker.StartRaftNodes(edgraph.State.WALstore, bindall, func() {

go func() { worker.Start...; edgraph.ResetAcl(); go edgraph.RefreshAcls() }

dgraph/cmd/alpha/run.go, line 517 at r1 (raw file):

		edgraph.ResetAcl()
	})
	go edgraph.RetrieveAclsPeriodically(shutdownCh)

This can lie in the go func above.

edgraph/access_ee.go, line 24 at r1 (raw file):

	"github.com/dgraph-io/dgraph/protos/pb"

Spaces between imports can be removed.

edgraph/access_ee.go, line 123 at r1 (raw file):

				"user not found for id %v", userId)
		}
	} else {

The above can return. So, this can come out of else.

edgraph/access_ee.go, line 125 at r1 (raw file):

	} else {
		var err error
		if ctx, err = appendAdminJwt(ctx); err != nil {

We shouldn't be using admin JWT. Just a boolean header would do if used right.

edgraph/access_ee.go, line 261 at r1 (raw file):

// query the user with the given userid, and returns associated uid, acl groups,
// and whether the password stored in DB matches the supplied password
func (s *Server) queryUser(ctx context.Context, userid string, password string) (user *acl.User,

authorizeUser

edgraph/access_ee.go, line 293 at r1 (raw file):

			return
		case <-ticker.C:
			if err := (&Server{}).RetrieveAcls(); err != nil {

Put RetrieveAcl func just above this periodic func.

edgraph/access_ee.go, line 362 at r1 (raw file):

// add an admin jwt to the context so that we can perform cluster internal operations such as
// retrieving acls from other servers
func appendAdminJwt(ctx context.Context) (context.Context, error) {

Can just be a simple header, saying dgraph-internal=true. As long as we strip it away when we receive a context from public endpoints.

Update: This , or even the header isn't required. Just refactor Query to use an internal func.

edgraph/access_ee.go, line 375 at r1 (raw file):

// retrieve the full data set of ACLs from the corresponding alpha server, and update the aclCache
func (s *Server) RetrieveAcls() error {

Can be moved internal to the periodic func.

edgraph/access_ee.go, line 386 at r1 (raw file):

		return fmt.Errorf("unable to append admin jwt:%v", err)
	}
	queryResp, err := s.Query(ctx, &queryRequest)

Should not call the public Query endpoint here. Instead, you can refactor the code so all the authorization etc. happens in the public function, and then it calls the internal function; and you can call the internal func here without any extra "admin JWT" or special headers.

edgraph/access_ee.go, line 412 at r1 (raw file):

// extract the userId, groupIds from the accessJwt in the context
func extractUserAndGroups(ctx context.Context) (string, []string, error) {

You can just return one slice. With the idea that the first entry is user, and the rest are groupids. Easier to read the function then. Looks like that change would need to happen in validateToken.

In general, try to reduce the number of arguments to the func and the number of return args, makes for a nicer code read.

edgraph/access_ee.go, line 428 at r1 (raw file):

// parse the Schema in the operation and authorize the operation using the aclCache
func (s *Server) parseAndAuthorizeAlter(ctx context.Context, op *api.Operation) (bool,

Just return error. We can do the schema.Parse again later, it's alright.

authorizeAlter

edgraph/access_ee.go, line 470 at r1 (raw file):

// authorize the mutation using the aclCache
func (s *Server) authorizeMutation(ctx context.Context, gmu *gql.Mutation) error {

None of these need to be "attached" to Server struct. So, can be outside.

authorizeMutate

edgraph/access_ee.go, line 490 at r1 (raw file):

// authorize the query using the aclCache
func (s *Server) authorizeQuery(ctx context.Context, parsedReq gql.Result) error {

I'd say both for mutation and query, generate a list of predicates that you care about. Then, check if these preds have the right access, given the groups, etc.

worker/groups.go, line 65 at r1 (raw file):

// This function triggers RAFT nodes to be created, and is the entrace to the RAFT
// world from main.go.
func StartRaftNodes(walStore *badger.DB, bindall bool, callback func()) {

No need for callback.

+// expiryTime returns a UTC based expiry time using TTL.
+func expiryTime(ttl time.Duration) int64 {

•   return time.Now().UTC().Add(Config.AccessJwtTtl).Unix()
  

+}

Rename AclBytesToMap: acl.UnmarshalAcl(...)

Reviewable status: 0 of 23 files reviewed, 20 unresolved discussions (waiting on @gitlw, @manishrjain, @danielmai, @codexnull, and @srfrog)

Regarding the expiryTime function, there are different ttl configs for access jwt and refresh jwt, so a common function for both is not applicable.

Reviewable status: 0 of 20 files reviewed, 21 unresolved discussions (waiting on @manishrjain, @danielmai, @codexnull, and @srfrog)

dgraph/cmd/alpha/run.go, line 126 at r1 (raw file):

Done.

dgraph/cmd/alpha/run.go, line 128 at r1 (raw file):

Done.

dgraph/cmd/alpha/run.go, line 130 at r1 (raw file):

Done.

dgraph/cmd/alpha/run.go, line 132 at r1 (raw file):

Done.

dgraph/cmd/alpha/run.go, line 407 at r1 (raw file):

Not changing per the later comment that I should leave it as it is.

dgraph/cmd/alpha/run.go, line 512 at r1 (raw file):

Done.

dgraph/cmd/alpha/run.go, line 517 at r1 (raw file):

Done.

edgraph/access_ee.go, line 24 at r1 (raw file):

Done.

edgraph/access_ee.go, line 123 at r1 (raw file):

Done.

edgraph/access_ee.go, line 125 at r1 (raw file):

As I think about it, if we think the cluster internal traffic is protected from hostile environment, then it makes no difference whether we add the header or not. So I simply removed the admin jwts.

edgraph/access_ee.go, line 261 at r1 (raw file):

Done.

edgraph/access_ee.go, line 293 at r1 (raw file):

Done.

edgraph/access_ee.go, line 362 at r1 (raw file):

Done.

edgraph/access_ee.go, line 375 at r1 (raw file):

Done.

edgraph/access_ee.go, line 386 at r1 (raw file):

Done.

edgraph/access_ee.go, line 412 at r1 (raw file):

Done.

edgraph/access_ee.go, line 428 at r1 (raw file):

Done.

edgraph/access_ee.go, line 470 at r1 (raw file):

Done.

edgraph/access_ee.go, line 490 at r1 (raw file):

Done.

edgraph/access_ee.go, line 505 at r2 (raw file):

I'd say both for mutation and query, generate a list of predicates that you care about. Then, check if these preds have the right access, given the groups, etc.

Very close to ready. Just a few more comments on code and testing.

Reviewable status: 0 of 20 files reviewed, 25 unresolved discussions (waiting on @gitlw, @danielmai, @codexnull, and @srfrog)

edgraph/access_ee.go, line 106 at r3 (raw file):

				request.RefreshToken, err)
		}

Check all your bases.

x.AssertTrue(len(userData) > 0)

Don't use x.AssertTruef for this path, because the args are expensive.

edgraph/access_ee.go, line 119 at r3 (raw file):

		return user, nil
	} else {

You can drop the else.

edgraph/access_ee.go, line 176 at r3 (raw file):

			groupId, ok := group.(string)
			if !ok {
				glog.Errorf("unable to convert group to string:%v", group)

It doesn't continue or return? Shouldn't this return an error, or refresh the token or something?

edgraph/access_ee.go, line 277 at r3 (raw file):

}

func RefreshAcls(closeCh <-chan struct{}) {

Add a TODO to convert the closeCh this to a y.Closer. We should also switch the stopChan to y.Closer.

edgraph/access_ee.go, line 286 at r3 (raw file):

	defer ticker.Stop()

	// retrieve the full data set of ACLs from the corresponding alpha server, and update the aclCache

100 chars.

edgraph/access_ee.go, line 287 at r3 (raw file):

	// retrieve the full data set of ACLs from the corresponding alpha server, and update the aclCache
	RetrieveAcls := func() error {

Can be small case, to indicate private method. For consistency.

edgraph/access_ee.go, line 288 at r3 (raw file):

	// retrieve the full data set of ACLs from the corresponding alpha server, and update the aclCache
	RetrieveAcls := func() error {
		glog.Infof("Retrieving ACLs")

Refreshing ACLs. Also, put it at glog.V(1).Infof

edgraph/access_ee.go, line 316 at r3 (raw file):

			aclCache.Store(group.GroupID, &group)
		}
		glog.Infof("updated the ACL cache with %d entries", storedEntries)

glog.V(1).Infof(...)

edgraph/access_ee.go, line 412 at r3 (raw file):

	}

	updates, err := schema.Parse(op.Schema)

Looks like updates are being iterated over later. In that case, move this closer to usage.

edgraph/access_ee.go, line 417 at r3 (raw file):

	}

	userData, err := extractUserAndGroups(ctx)

if userData, err := ...; err != nil {
  ...
} else if userData[0] == "admin" {
  ...
}

Easier to read. Also, tells you that the var isn't being used below.

edgraph/access_ee.go, line 429 at r3 (raw file):

	// if we get here, we know the user is not admin
	if op.DropAll {
		return fmt.Errorf("only the admin is allowed to drop all predicates")

s/drop all data

edgraph/access_ee.go, line 449 at r3 (raw file):

}

func populatePredsFromMutation(nquads []*api.NQuad, preds map[string]struct{}) {

func parsePreds(nquads) map[string]struct{} {
  returns a map
}

edgraph/access_ee.go, line 478 at r3 (raw file):

	}

	preds := make(map[string]struct{})

Make the func do a bit more, by returning a map.

edgraph/access_ee.go, line 482 at r3 (raw file):

	groupIds := userData[1:]
	for pred := range preds {

for pred := range parsePreds(...)

edgraph/access_ee.go, line 490 at r3 (raw file):

}

func populatePredsFromQuery(gqls []*gql.GraphQuery, preds map[string]struct{}) {

Can return preds map.

Unless you're passing in a partially filled map.

edgraph/access_ee.go, line 518 at r3 (raw file):

	userId := userData[0]
	if userId == "admin" {

I don't see userId being used anywhere. Why not if userData[0] == "admin".

Better still, create a small func, func isAdmin(userData []string) bool { ... }. So, it can also check if the length is zero.

edgraph/access_ee.go, line 545 at r3 (raw file):

}

func authorizePredicate(groups []string, predicate string, operation int32) bool {

If you can return an error instead of a bool, that'd almost always work better. Because it allows us to send back very specific error messages, so the user gets to understand what failed. See examples below.

edgraph/access_ee.go, line 559 at r3 (raw file):

	entry, found := aclCache.Load(groupId)
	if !found {
		glog.Infof("acl not found")

return fmt.Errorf("ACL for group: %s not found", groupId)

edgraph/access_ee.go, line 564 at r3 (raw file):

	aclGroup := entry.(*acl.Group)
	perm, found := aclGroup.MappedAcls[predicate]
	allowed := found && (perm&operation) != 0

if !allowed {
  return fmt.Errorf("group: %s not allowed to do X on predicate: %s", ...)
}

edgraph/server.go, line 291 at r3 (raw file):

	}

	err := authorizeAlter(ctx, op)

if err := ...; err != nil

Learn this by heart, add a shortcut, or make your IDE remind you of this. It's the Go way.

edgraph/server.go, line 457 at r3 (raw file):

}

func (s *Server) Query(ctx context.Context, req *api.Request) (resp *api.Response, err error) {

You can probably remove the return var names (resp and err).

ee/acl/acl_test.go, line 36 at r3 (raw file):

	userid         = "alice"
	userpassword   = "simplepassword"
	alphaPort      = 9280

This is different from 9180 below? That's an Alpha port. I'd say keep it the same. And have @codexnull 's testing framework deal with getting the right cluster in place.

ee/acl/docker-compose.yml, line 14 at r3 (raw file):

      - 5080:5080
      - 6080:6080
    command: /gobin/dgraph zero --my=zero1:5080 --replicas 3 --idx 1 --bindall --expose_trace --profile_mode block --block_rate 10 --logtostderr -v=2

Set replicas to 1, and test a 3-node cluster. 1 Zero, 2 alphas, i.e. 2 groups. Make sure that you hit both the Alphas in your testing.

ee/acl/docker-compose.yml, line 38 at r3 (raw file):

      - 8180:8180
      - 9180:9180
      - 9999:9999

What's 9999 for?

Reviewed 1 of 14 files at r2, 1 of 20 files at r3, 2 of 6 files at r4, 17 of 19 files at r5, 4 of 5 files at r6, 2 of 2 files at r7.
Reviewable status: all files reviewed, 28 unresolved discussions (waiting on @manishrjain, @golangcibot, @danielmai, @codexnull, and @srfrog)

edgraph/access_ee.go, line 106 at r3 (raw file):

Done.

edgraph/access_ee.go, line 119 at r3 (raw file):

Done.

edgraph/access_ee.go, line 176 at r3 (raw file):

Good catch.

edgraph/access_ee.go, line 277 at r3 (raw file):

Done.

edgraph/access_ee.go, line 286 at r3 (raw file):

Done.

edgraph/access_ee.go, line 287 at r3 (raw file):

Done.

edgraph/access_ee.go, line 288 at r3 (raw file):

Done.

edgraph/access_ee.go, line 316 at r3 (raw file):

Done.

edgraph/access_ee.go, line 412 at r3 (raw file):

Done.

edgraph/access_ee.go, line 417 at r3 (raw file):

But I cannot seem to do it after putting the x.AssertTrue(len(userData) > 0) check.

edgraph/access_ee.go, line 429 at r3 (raw file):

Done.

edgraph/access_ee.go, line 449 at r3 (raw file):

Done.

edgraph/access_ee.go, line 478 at r3 (raw file):

Done.

edgraph/access_ee.go, line 482 at r3 (raw file):

Done.

edgraph/access_ee.go, line 490 at r3 (raw file):

Done.

edgraph/access_ee.go, line 518 at r3 (raw file):

func isAdmin(userData []string) bool { ... }

Done

edgraph/access_ee.go, line 545 at r3 (raw file):

Done.

edgraph/access_ee.go, line 559 at r3 (raw file):

Done.

edgraph/access_ee.go, line 564 at r3 (raw file):

Done.

edgraph/access_ee.go, line 582 at r4 (raw file):

Done.

edgraph/server.go, line 291 at r3 (raw file):

I can try to remember this but I'm not sure how to make my IDE remind of this.
Also I'm not sure about the reasoning that the inlined version is better than the not inlined one, because as soon as the function is returning multiple variables, the error checking shouldn't be inlined.
e.g.
x, err := f()
if err != nil {
// error handling
return
}
// use x
as suggested in https://github.com/golang/go/wiki/CodeReviewComments#indent-error-flow

Also the very first example in the "Error handling" page of go is doing a non-inlined error checking
https://blog.golang.org/error-handling-and-go

In any case, if you think the inlined version is better, I'll try best to avoid the non inlined style.

edgraph/server.go, line 457 at r3 (raw file):

Done.

ee/acl/acl_test.go, line 36 at r3 (raw file):

I removed this variable as it's not being used any more.

ee/acl/docker-compose.yml, line 14 at r3 (raw file):

Done.

ee/acl/docker-compose.yml, line 38 at r3 (raw file):

It's the port I once used to attach the dlv debugger to the alpha inside a container. I've removed it.

contrib/integration/acctupsert/main.go, line 128 at r4 (raw file):

Done.

contrib/integration/acctupsert/main.go, line 142 at r4 (raw file):

Done.

I pushed changes from my review. Just make sure that we don't depend on a test package in dgo for testing. And also check that the groot account creation works correctly, if multiple alphas start up at the same time.

Do have a look at the changes I made. Rest is good.

Reviewable status: 15 of 22 files reviewed, 8 unresolved discussions (waiting on @gitlw, @manishrjain, @golangcibot, @danielmai, @codexnull, and @srfrog)

edgraph/access.go, line 42 at r8 (raw file):

func RefreshAcls(closer *y.Closer) {
	// do nothing

I made a change so we are dealing correctly with closer.

edgraph/access_ee.go, line 414 at r8 (raw file):

	aclCache = sync.Map{}
	if err := upsertAdmin(context.Background()); err != nil {

What happens if admin creation fails, due to some other reason (apart from txn conflict)? Shouldn't you repeat the txn, until you can either find the admin, or your txn succeeds?

edgraph/server.go, line 291 at r3 (raw file):

I think for multiple return vars, including error: Yeah, you might want to put them in multiple lines. But, for single error response, which you intent to use for return, it's a common thing to do:

https://golang.org/doc/effective_go.html#if

Reviewable status: 15 of 22 files reviewed, 8 unresolved discussions (waiting on @gitlw, @manishrjain, @golangcibot, @danielmai, @codexnull, and @srfrog)